@pkaminski reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on snoack).

.github/workflows/check-build-output.yaml line 18 at r1 (raw file):

        with:
          node-version: "24"
          cache: yarn

At least Codex was convinced that actions/setup-node was incapable of caching Yarn 4 correctly, and explicit use of actions/cache was required. Are you sure this works? If so, should backport to a bunch of other workflows.

@snoack made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on pkaminski).

.github/workflows/check-build-output.yaml line 18 at r1 (raw file):

Well, Codex came up with this simplification, and asking it about it again now, it insists that this works with Yarn 4 since actions/setup-node internally uses actions/cache. I did no test it. I suppose once this has been merged we can rerun the CI for the integration changes in work-diary and update-changelog-action and see if it works.

@pkaminski made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on snoack).

.github/workflows/check-build-output.yaml line 18 at r1 (raw file):

How would we know it's working, though?

The Yarn maintainers seem to think (or did, 5 years ago) that caching node_modules and .yarn/install-state.gz would often be beneficial for our configuration.

@snoack made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on pkaminski).

.github/workflows/check-build-output.yaml line 18 at r1 (raw file):

How would we know it's working, though?

Besides making sure the CI passes, we watch for cache related log messages.

The Yarn maintainers seem to think (or did, 5 years ago) that caching node_modules and .yarn/install-state.gz would often be beneficial for our configuration.

The premise is that by caching node_modules and .yarn/install-state.gz instead of caching the Yarn cache you eliminate overhead due to compression (disabled by default in Yarn 4) and bookkeeping in Yarn. We already invalidate the cache on dependency changes, so we wouldn't lose cachability of packages across CI runs with changed dependencies — we never had that. Note sure how significant an optimization this is in 2026 with Yarn 4 though, and the downside would be a slightly more complex workflow configuration since I would have to reintroduce a separate actions/cache step.

Also if we are concerned about yarn install (from Yarn cache) overhead, work-diary and update-changelog-action aren't the largest offenders; reviewable-server and reviewable-client have a much larger npm dependency footprint and caching the Yarn cache is consistent with what we are doing there.

@pkaminski reviewed 1 file and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on snoack).

.github/workflows/check-build-output.yaml line 18 at r1 (raw file):

Yeah, I'm not concerned about these internal tool packages, just using this as a platform to figure out the best approach to use consistently across our codebase.

I think the simple approach here is better and Codex misled me before. Let's go with that throughout — could you see if Codex will undo the mess it made? Thanks.

@snoack made 1 comment.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on snoack).

.github/workflows/check-build-output.yaml line 18 at r1 (raw file):

There is a problem with this approach though: Before corepack enable, yarn is Yarn 1.x installed by default on the runner. So we have some kind of a circular dependency here: corepack enable requires actions/setup-node which requires the right Yarn version (installed via corepack) if configured with cache: yarn. I suppose we have to revert to an explicit caching step.

@pkaminski reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on snoack).

.github/workflows/check-build-output.yaml line 18 at r1 (raw file):

ChatGPT's suggestion was to run setup-node twice, don't know if that's better or worse:

- uses: actions/checkout@v6

- uses: actions/setup-node@v6
  with:
    node-version: 24

- run: corepack enable

- uses: actions/setup-node@v6
  with:
    node-version: 24
    cache: yarn
    cache-dependency-path: |
      yarn.lock
      .yarnrc.yml

- run: yarn install --immutable

@snoack made 1 comment and resolved 1 discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on snoack).

.github/workflows/check-build-output.yaml line 18 at r1 (raw file):

I suppose that could work, but I can't say that I like it any better. Also what I have implemented now is 100% consistent with what we are already doing everywhere else.